Description

[BUG FIX]

The second keyrecord_t* argument given to tap hold per key functions should refer to the same key as that given in the first uint16_t keycode argument. All the per-key functions called in bool process_tapping(keyrecord_t *keyp) use the same pattern : get_option(tapping_keycode, keyp) but from what I've gathered, keyp is not necessarily the tapping_key so it should instead be get_option(tapping_keycode, &tapping_key).

To illustrate my point:

Let the key on row=1, col=3 be MT(MOD_LSFT, KC_S) (hexadecimal keycode=0x6216 ) and the key on row=2, col=7 be KC_H.
Consider the following body for the get_tapping_term function.

uint16_t get_tapping_term(uint16_t keycode, keyrecord_t *record) {
    uprintf("0x%04X,%u,%u,%u,%u\n",
         keycode,
         record->event.key.row,
         record->event.key.col,
         get_highest_layer(layer_state),
         record->event.pressed
         );
    return TAPPING_TERM;
}  

If we try to capitalise H by holding down the S shift modtap, we can see the following appear in the printed output:

0x6216,1,3,0,1
0x6216,2,7,0,1
0x6216,1,3,0,1

The second printed line shows the keycode of the mod-tap but the keyrecord_t information of KC_H
Isn't keycode and record supposed to refer to the same underlying key event?

You can't even really exploit it that much, it seems. I thought that perhaps this might be useful to tweak the tapping term of a specific modtap if it is combined with another key but the following doesn't increase the tapping term for LSFT_T(KC_S) when its tap is interrupted by KC_H (row=2, col=7)

uint16_t get_tapping_term(uint16_t keycode, keyrecord_t *record) {
    uprintf("0x%04X,%u,%u,%u,%u\n",
         keycode,
         record->event.key.row,
         record->event.key.col,
         get_highest_layer(layer_state),
         record->event.pressed
         );
    switch (keycode) {
        case LSFT_T(KC_S):
            if (record->event.key.row == 2 && record->event.key.col == 7) {
                return 8000;
            } else {
                return TAPPING_TERM-10;
            }
        default:
            return TAPPING_TERM;
    }
};

As in, I most definitely do not need to be holding down LSFT_T(KC_S) for 8s before pressing KC_H in order to capitalise it.

When discussing this issue on the Discord server, sigprof also pointed out instances where the get_tapping_term function is called with a NULL second argument in core QMK code which presents the high risk of nullptr dereference at runtime if a user, for example, dereferences the record argument to fetch the keypos information in their per-key tap hold function(s). To avoid this problem, I replaced the NULLs by an empty keyrecord structure (&(keyrecord_t){}). Let me know if there's a better substitute that I can use.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).